feat: Add capture buffer for unknown/error bus frames with web UI page#44
Conversation
|
Great idea, I'll have a look over and get it merged in. |
marklynch
left a comment
marklynch
left a comment
There was a problem hiding this comment.
Review: capture buffer for unknown/error bus frames
Overall: Solid, well-contained feature. Good mutex discipline, OOM checks throughout the JSON handler, XSS-safe DOM construction (textContent, not innerHTML), a host-test stub so the suite still links, a CHANGELOG entry, and the HTTP_MAX_URI_HANDLERS bump (14→20) correctly covers the 3 new endpoints. The payload offset math (raw[10]…raw[len-3]) matches the real frame layout in message_decoder.c. ✅
Suggestions (low severity — none blocking)
-
unknown_buffer_record()is called while holding the pool-state mutex (message_decoder.c, themessages_unknown_total++branch). Every other call site releasesstate_mutexfirst; here it runs an O(capacity × 64-byte memcmp) dedup scan inside the critical section. There's no deadlock (it takes its own separate mutex), but it needlessly extends the pool-state critical section — worth moving the call afterxSemaphoreGive(ctx->state_mutex)for consistency with the other sites. -
Transient heap pressure in the JSON handler.
unknown_msgs_json_handlerdoesmalloc(UNKNOWN_BUFFER_CAPACITY * sizeof(unknown_entry_t))≈ ~9 KB per request, on top of the ~9 KB statics_entries[]. It's guarded with an OOM check so it degrades gracefully, but you could avoid the copy entirely by streaming entries to the response under the lock (or reduce capacity). The new free-heap readout on the Status page is handy for keeping an eye on this. -
Dedup collapses distinct long frames. Dedup keys on
raw_len == len && memcmp(first store_len bytes), so two frames longer than 64 bytes that share their first 64 bytes and length merge into one entry. Acceptable given the truncation design — just worth a one-line comment noting it. -
Minor cleanup in
unknown_msgs_json_handler.stored_lenandstore_lencompute the samemin(raw_len, MAX_RAW_BYTES)cap twice and could be a single variable. Also the inline frame-layout comment lists[CMD=7][LEN=8]...but skips[HDR_CHK=9]beforeDATA=10— the offset is correct, the comment is just incomplete vs. the canonical one inmessage_decoder.c.
Nice utility overall — the truncation handling and "first N bytes of M" display in the UI are a nice touch.
Generated by Claude Code
|
@lawther I did a Claude review above as don't have my computer with me. The keys one for me is point 1 due to previous scars from mutex locking, and the comment on point 3. If you could have a look at those I would appreciate it. Great improvement. |
lawther
force-pushed
the
feat/buffer-unknowns
branch
from
9a745f4 to
8112350
Compare
Captures frames that have no registered decoder (unknown) or that fail checksum/framing validation (error) into a fixed-capacity ring buffer with LFU eviction. Dedup is keyed on error status, length, and the first UNKNOWN_BUFFER_MAX_RAW_BYTES (64) bytes — frames that only differ beyond byte 64 collapse into one entry, which is a known limitation. Exposes the buffer via two HTTP endpoints: GET /unknown-msgs-json — JSON dump used by the web UI POST /unknown-msgs-clear — clears the buffer The web UI page at /unknown-msgs lists entries sorted by most-recently- seen, showing src/dst/cmd, decoded device names, hit count, timestamps, payload bytes, and the raw frame. Implementation notes: - unknown_buffer uses its own mutex; lock_for_read()/unlock_after_read() let the JSON handler iterate s_entries directly without a heap copy. - Hex strings use a single 193-byte stack buffer (MAX_RAW_BYTES * 3 + 1) reused for payload and raw fields; cJSON copies on each Add call. - unknown_buffer_record() is called after xSemaphoreGive(state_mutex) at all call sites to keep the pool-state critical section short.
lawther
force-pushed
the
feat/buffer-unknowns
branch
from
8112350 to
5222737
Compare
|
Thanks for the review Mark, I've addressed all the points. ptal.
Good catch. Done, it's neater this way too.
Fair point. I originally went for the 'hold lock for minimal time' approach, but now I've taken the 'stream entries under lock' approach. If the lock can't be acquired, it returns a HTTP 500 with a message to retry. Given the way that lock acquisition waits up to 100ms, things would have to be pretty overloaded for this to happen.
Comment added. I did originally consider individually mallocing storage for each unknown frame as it came in to ensure proper storage and dedupe. I thought it was overly fraught, and any fragmentation from lots of small mallocs would probably eat into any savings. I'm not 100% sure how the allocator works. So I came down on the side of a static block, taking ~9KB, which I thought was a worthwhile tradeoff.
All done. |
|
Thanks for fixing up. Merged in |
2 participants
I added a little utility to cache unknown/bad messages so I didn't have to be capturing logs all the time.